feat(Masthead): Update subcomponent names #729
Conversation
wise-king-sullyman
force-pushed
the
masthead-renames
branch
from
7950b85 to
36e7225
Compare
| import { JSXOpeningElement, JSXMemberExpression } from "estree-jsx"; | ||
|
|
||
| /** Gets the name of an opening element or member expression */ | ||
| export function getNodeName(node: JSXOpeningElement | JSXMemberExpression) { |
There was a problem hiding this comment.
| export function getNodeName(node: JSXOpeningElement | JSXMemberExpression) { | |
| export function getNodeName(node: JSXOpeningElement) { |
I'd rather separate the JSXMemberExpression handling into a separate function. Because if I am not mistaken, getNodeName will be called from the outside only with a node of type JSXOpeningElement, but when calling it recursively from the inside, we always pass a JSXMemberExpression node.
We can have it as a private function in this file I think.
function getJSXMemberExpressionName(node: JSXMemberExpression) {
switch (node.object.type) {
case "JSXMemberExpression":
return getJSXMemberExpressionName(node.object);
case "JSXIdentifier":
return node.object.name;
}
}
There was a problem hiding this comment.
So my idea with just naming this helper getNodeName was that over time we would expand it for resolving the name of nodes of various types.
I'm not against splitting the logic into discrete private functions and minimizing recursion to help improve readability at all though 🙂
CC @thatblindgeye since you left a comment about this same kind of stuff.
| // dist imports | ||
| { | ||
| code: `import { MastheadBrand } from '@patternfly/react-core/dist/esm/components/Masthead/MastheadBrand'; <MastheadBrand />`, | ||
| output: `import { MastheadLogo } from '@patternfly/react-core/dist/esm/components/Masthead/MastheadBrand'; <MastheadLogo data-codemods />`, |
There was a problem hiding this comment.
Shouldn't the import path be changed to .../Masthead/MastheadLogo ? I am not sure about this, just asking.
| @@ -0,0 +1,18 @@ | |||
| ### masthead-name-changes [(#10809)](https://github.com/patternfly/patternfly-react/pull/10809) | |||
|
|
|||
| The old `MastheadBrand` component has been renamed to `MastheadLogo`, and the old `MastheadMain` has been renamed to `MastheadBrand`. | |||
There was a problem hiding this comment.
I think the message might be confusing for some consumers (or at least the messages given in the codemod output "MastheadMain has been renamed to MastheadBrand"), I like using the word "old" here, which makes more sense.
I was confused at the first place why are we renaming these components, when they will still stay in the new Masthead.
Maybe we can add a sentence here, like "MastheadMain will now wrap both MastheadToggle and MastheadBrand within itself.".
I know there will probably be a second codemod for wrapping the components in MastheadMain and the message there will better describe the whole change, but still this renaming which is not really renaming might be confusing (but probably not many consumers read the messages, I guess they just run the fix, so I am ok with keeping it as is).
| import { ImportDefaultSpecifierWithParent } from "./interfaces"; | ||
|
|
||
| /** Gets the import path string for a default import */ | ||
| export function getDeclarationString( |
There was a problem hiding this comment.
More of a nit so not a blocker, but would getDefaultDeclarationString or something similar be more accurate?
| import { JSXOpeningElement, JSXMemberExpression } from "estree-jsx"; | ||
|
|
||
| /** Gets the name of an opening element or member expression */ | ||
| export function getNodeName(node: JSXOpeningElement | JSXMemberExpression) { |
There was a problem hiding this comment.
Going off of Adam's comment above, another nit so not a blocker but if this helper is meant solely for these 2 scenarios, might be worth updating the name to reflect that (if we can without it become tooooo verbose).
| import { getAttributeName } from "./getAttributeName"; | ||
|
|
||
| /** Returns true if the passed opening element has a data-codemods attribute */ | ||
| export function hasCodeModDataTag(openingElement: JSXOpeningElement) { |
There was a problem hiding this comment.
Could always be a separate helper, but just wondering if we should only check if the data tag exists by name, or return the actual attribute and base conditional logic on that. Just thinking if we ever start supplying specific values to a data-codemods attribute, e.g. maybe we only want to skip/run a rule if an openingElement has data-codemods="renamed", but not data-codemods="deprecated" or something.
| @@ -0,0 +1,7 @@ | |||
| import { MastheadLogo, MastheadBrand } from "@patternfly/react-core"; | |||
There was a problem hiding this comment.
This output just needs to be re-updated, looks like it's still using the fixed output from when you were also updating MastheadMain to MastheadBrand
|
|
||
| if (shouldRenameNode) { | ||
| fixes.push(fixer.replaceText(node.name, newName)); | ||
| fixes.push(fixer.insertTextAfter(node.name, " data-codemods")); |
There was a problem hiding this comment.
I wonder if this should be optional, I know we need the data-codemods for the followup Masthead rule, but in other cases it may be an unnecessary addition.
But if anything, it can be done as a followup.
There was a problem hiding this comment.
Yeah I went back and forth on that and IIRC talked it over with @thatblindgeye, I think we landed on always adding it to prevent issues and adding #723 so that we aren't leaving consumer codebases littered with our data tags.
I'm not super opposed to making it optional though, but yeah would want to do it as a followup.
Towards #718